Add TCPKeepAlive to SqlClient Sockets #33024

AfsanehR-zz · 2018-10-24T22:25:19Z

Fixes #28061.

…pAliveFix

stephentoub · 2018-10-25T02:20:40Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.cs

+#if netcoreapp
+                        // enable keep-alive on socket
+                        const int time = 30;
+                        const int interval = 1000;


These probably don't need to be constants and can just be passed directly into the method calls... the names here don't really add anything. If we do keep the consts, though, they should be PascalCased.

AfsanehR-zz · 2018-10-25T17:22:29Z

CI tests are failing on All configurations:

System\Data\SqlClient\SNI\SNITcpHandle.cs(231,92): error CS0117: 'SocketOptionName' does not contain a definition for 'TcpKeepAliveInterval' [D:\j\workspace\windows-TGrou---0d2c9ac4\src\System.Data.SqlClient\src\System.Data.SqlClient.csproj]

System\Data\SqlClient\SNI\SNITcpHandle.cs(232,92): error CS0117: 'SocketOptionName' does not contain a definition for 'TcpKeepAliveTime' [D:\j\workspace\windows-TGrou---0d2c9ac4\src\System.Data.SqlClient\src\System.Data.SqlClient.csproj]

Given from the configurations in System.Net.Sockets.csproj I see it's configured to these settings:
<Configurations>netcoreapp-Unix-Debug;netcoreapp-Unix-Release;netcoreapp-Windows_NT-Debug;netcoreapp-Windows_NT-Release;uap-Windows_NT-Debug;uap-Windows_NT-Release</Configurations>

@stephentoub @weshaggard could you please confirm if I had taken the right approach?
I have added the netcoreapp specific tags where these SocketOptionNames are used, I also added UAPAssembly in the last commit. Local build -allConfigurations was successful, I will check again once CI runs are finished.

AfsanehR-zz · 2018-10-25T18:50:52Z

@dotnet-bot test Linux arm64 Release Build

stephentoub · 2018-10-25T20:26:59Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.cs

@@ -223,6 +223,16 @@ void Cancel()
                    if (ipAddresses[i] != null)
                    {
                        sockets[i] = new Socket(ipAddresses[i].AddressFamily, SocketType.Stream, ProtocolType.Tcp);
+#if netcoreapp
+#if uapassembly


You're only doing this if this is both netcoreapp and uapassembly?

yes, I am not sure if this is the proper way though, I need some guidance. As I mentioned, the configurations in System.Net.Sockets.csproj are set for netcoreapp and uap only.

@afsanehr I think you only need the #if netcoreapp, here. This code will only be compiled when SqlClient is running on .Net Core on UWP. And not for other platforms.

@saurabh500 I originally had it for netcoreapp and the AllConfigurations build was failing for these error messages still:
System\Data\SqlClient\SNI\SNITcpHandle.cs(231,92): error CS0117: 'SocketOptionName' does not contain a definition for 'TcpKeepAliveInterval' [D:\j\workspace\windows-TGrou---0d2c9ac4\src\System.Data.SqlClient\src\System.Data.SqlClient.csproj]

Link to failing CI using netcoreapp only: https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_all+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/17322/

There is not an answer that will solve all the scenarios but we generally use netcoreapp and let it float and when we remap netcoreapp to the next version we generally leave behind a specific version for the older version as necessary and it is at that time we many need to adjust the defines to also include the older version.

For reference our guidance is at https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/project-guidelines.md#conventions-for-forked-code.

We could have the feature flag for netcoreapp, and going forward, any netcoreapp version would definitely have these APIs in there.

Yes you could define that layering in your project. However we generally haven't made netcoreapp also imply uap which is why I suggested feature define and how you determine to define that in your project is up to you.

Got it. The UAP here is unnecessary and should be removed. This will end up being a DefineConstant with a TargetGroup == netcoreapp check.

Thanks @weshaggard

saurabh500 · 2018-10-26T02:00:25Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.cs

+#if netcoreapp
+#if uapassembly
+                        // enable keep-alive on socket
+                        const int Time = 30;


You can get rid of these const and simply inline the value in API call.

saurabh500 · 2018-10-26T16:15:39Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.cs

+                        // enable keep-alive on socket
+                        sockets[i].SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.KeepAlive, true);
+                        sockets[i].SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.TcpKeepAliveInterval, 1000);
+                        sockets[i].SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.TcpKeepAliveTime, 30);


This is in minutes, right?

@saurabh500 Good question .. I am trying to look for documentation . I see here it mentions in milliseconds:
https://docs.microsoft.com/en-us/previous-versions/windows/it-pro/windows-server-2003/cc782936(v=ws.10)

This user also mentioned the options are milliseconds here:
https://github.com/dotnet/corefx/issues/25040

We need 30 minutes. 30 Millis doesn't make sense. I wonder how Karl came up with this value. Can you sync with him?
Native SNI uses 30 minutes

According to these links:
1- https://msdn.microsoft.com/en-us/library/windows/desktop/dd877220%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396

2- http://man7.org/linux/man-pages/man7/tcp.7.html

The measurements for the TcpKeepAliveInterval and TcpKeepAliveTime in Windows and Linux OS are different. In Linux the TCP_KEEPINTVL and TCP_KEEPIDLE is set in seconds, while in Windows these values are measured in milliseconds.
Should we have different values based on different platform?
@stephentoub @karelz Do you know if this is something we can get some guidance from System.Net.sockets area owners?

Should we have different values based on different platform?
@stephentoub @karelz Do you know if this is something we can get some guidance from System.Net.sockets area owners?

No, otherwise it makes it really difficult to use. Since these are already different settings on the different platforms and the Sockets class is trying to provide an abstraction over them, it should also be normalizing the units. If it's not, that's a bug and we should fix it.

@afsanehr, can you open an issue tracking that? Thanks.

cc: @luigiberrettini

As highlighted in #25040 TCP_KEEPALIVE / TCP_KEEPIDLE and TCP_KEEPINTVL are expressed in seconds while SIO_KEEPALIVE_VALS are expressed in milliseconds.

Since #29963 allows to set keep-alive retry count, time and interval on all platforms, but without making the developer life easier, there is still the need to code the logic that allow to use the proper socket method depending on the operating system and its version:

// *** Retry count // Win 2000, Win XP, Win Server 2003 // - no change via socket methods // - value is 5 or min(255, max(TcpMaxDataRetransmissions, PPTPTcpMaxDataRetransmissions)) // Win Vista and later < Win 10 v1703 // - no change via socket methods // - value is 10 // Linux, OS X, Windows >= Win 10 v1703 socket.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.TcpKeepAliveRetryCount, RetryCount); // *** Time and interval // Windows < Win 10 v1709 socket.IOControl(IOControlCode.KeepAliveValues, byteArrayWithTimeAndIntervalMilliseconds, null); // Linux, OS X, Windows >= Win 10 v1709 socket.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.TcpKeepAliveTime, TimeSeconds); socket.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.TcpKeepAliveInterval, IntervalSeconds);

there is still the need to code the logic that allow to use the proper socket method depending on the operating system and its version:

For time and interval, why can't we just make this work from the .net layer? We'd just translate the SetSocketOption to the appropriate call on a lower version of Windows, and we'd ensure the. NET API is consistent on units.

Since IOControl accept a structure with on/off, time and interval, if a developer wants to set only time or interval, which value would you use for the other?
Moreover, when a user tries to set both with two different SetSocketOption calls, the second call will override what has been set by the first call.

I think we can either keep things as they are (every dev will have to write the logic) or create an API that expect a bool (enable/disable) and the three values to be passed: this would allow performing individual SetSocketOption or other calls under the hood.

@stephentoub created issue #33111 for tracking.

@afsanehr I want to correct my statement, the keep alive time should be 30 seconds. And the KeepAlive interval should be 1 second.
Values from Windows code in milliseconds.

DWORD const KEEPALIVETIME = 30000; DWORD const KEEPALIVEINTERVAL = 1000;

AfsanehR-zz · 2018-10-26T17:57:58Z

@dotnet-bot test Linux arm64 Release Build

AfsanehR-zz · 2018-10-26T18:39:47Z

@dotnet-bot test Linux arm64 Release Build

kburtram · 2018-10-31T22:44:11Z

@afsanehr we'd like to port this change to release/2.2 once it's merged. Do you know what we're waiting on?

I know for macOS\Linux the interval needs to be changed to 1, and given the current implementation of the Socket option, we'd need a conditional to use 30000\1000 on Windows, and then presumably a different branch for download level Windows clients to use IOControl. Please let me know if you would like help make these modifications, as we'd like to get this into release/2.2 as soon as possible.

stephentoub · 2018-11-01T00:15:44Z

we'd like to port this change to release/2.2 once it's merged

@kburtram, this change depends on #29963, which to my knowledge was not ported to release/2.2. Are you working with shiproom to port that to 2.2 as well? Or was it ported already and I missed it?
cc: @karelz

kburtram · 2018-11-01T03:57:50Z

@stephentoub we are working with shiproom to get the SqlClient and Sockets KeepAlive changes backported to release/2.2. I'll add you to the email thread for additional context and to provide feedback.

AfsanehR-zz · 2018-11-01T17:20:24Z

@kburtram Sure, I will update this today and will ask you for review.

saurabh500 · 2018-11-02T00:20:45Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.cs

@@ -223,6 +223,10 @@ void Cancel()
                    if (ipAddresses[i] != null)
                    {
                        sockets[i] = new Socket(ipAddresses[i].AddressFamily, SocketType.Stream, ProtocolType.Tcp);
+#if FEATURE_TCPKEEPALIVE


You can remove this If check. For other platforms, an empty function will be compiled.

saurabh500 · 2018-11-02T00:21:20Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.Windows.cs

+    internal partial class SNITcpHandle
+    {
+        internal static void SetKeepAliveValues(ref Socket socket)
+        {


Add a comment in the function saying why it has been left blank. Possible add the link to the issue, that we discussed should be opened.

saurabh500

Approved with some suggestions.

AfsanehR-zz · 2018-11-02T15:36:31Z

@dotnet-bot test Windows x86 Release Build
@dotnet-bot test Windows x64 Debug Build

AfsanehR-zz · 2018-11-02T15:43:19Z

@kburtram Discussed with @saurabh500 and we decided to add a class SNITcpHandle.unix to set the keepAlive values.
The SNITcpHandle.Windows class is empty for now and will update this for platform specific checks later. Reason is that ManagedSNI is enabled on Linux only, the original code was used on Linux only anyhow. This change should help with porting quickly, and we created an item to track the windows specific checks for later. #33209

AfsanehR-zz · 2018-11-02T20:16:19Z

@dotnet-bot test UWP NETNative x86 Release Build
@dotnet-bot test UWP CoreCLR x64 Debug Build
@dotnet-bot test Packaging All Configurations x64 Debug Build
@dotnet-bot test Linux arm64 Release Build

AfsanehR-zz · 2018-11-03T00:34:17Z

@dotnet-bot test NETFX x86 Release Build

AfsanehR-zz · 2018-11-03T03:44:29Z

@dotnet-bot test Windows x64 Debug Build

saurabh500 · 2018-11-03T22:35:31Z

@dotnet-bot test Windows x64 Debug Build

Timeout exceeded.

AfsanehR-zz · 2018-11-05T01:39:58Z

@dotnet-bot test Windows x64 Debug Build
@dotnet-bot test Windows x86 Release Build

Timeout exceeded.

AfsanehR-zz · 2018-11-05T21:54:35Z

@dotnet-bot test Windows x64 Debug Build
@dotnet-bot test Windows x86 Release Build

jinek · 2021-04-13T14:49:13Z

How to find original issue #28061 ?

stephentoub · 2021-04-13T23:58:44Z

https://github.com/dotnet/corefx/issues/28061, which is now dotnet/runtime#25458.

* Updating version files * Enable keep alive on SqlClient TCP sockets * make KeepAlive netcoreapp specific * added condition for uapassembly * added a new defineConstants called FEATURE_TCPKEEPALIVE * update TCPKeepAliveInterval value * set KeepAlive values in unix only * add documentation and add link to Github Issue * move SNITcpHandle.Windows.cs available in UAP * updating csproj for SNITcpHandle * fix in csproj for CI failure Commit migrated from dotnet/corefx@0db2274

dotnet-maestro bot and others added 5 commits October 24, 2018 12:00

Updating version files

5ba6be8

Enable keep alive on SqlClient TCP sockets

d627a21

Merge branch 'master' of https://github.com/dotnet/corefx into TCPKee…

22a310a

…pAliveFix

Merge branch 'master' of https://github.com/dotnet/corefx into TCPKee…

d149a2f

…pAliveFix

make KeepAlive netcoreapp specific

f7467b0

stephentoub reviewed Oct 25, 2018

View reviewed changes

added condition for uapassembly

73c4a57

AfsanehR-zz requested a review from saurabh500 October 25, 2018 17:22

stephentoub reviewed Oct 25, 2018

View reviewed changes

saurabh500 reviewed Oct 26, 2018

View reviewed changes

added a new defineConstants called FEATURE_TCPKEEPALIVE

b2c4a9b

saurabh500 approved these changes Oct 26, 2018

View reviewed changes

AfsanehR-zz added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Data.SqlClient labels Oct 26, 2018

AfsanehR-zz added 2 commits November 1, 2018 14:05

update TCPKeepAliveInterval value

f4519f3

set KeepAlive values in unix only

261eb30

saurabh500 reviewed Nov 2, 2018

View reviewed changes

saurabh500 approved these changes Nov 2, 2018

View reviewed changes

add documentation and add link to Github Issue

3a5b18a

saurabh500 removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 2, 2018

AfsanehR-zz added 3 commits November 2, 2018 15:03

move SNITcpHandle.Windows.cs available in UAP

9bf5e5f

updating csproj for SNITcpHandle

b9cd903

fix in csproj for CI failure

e2b2f6e

kburtram mentioned this pull request Nov 3, 2018

Add TCPKeepAlive to SqlClient Sockets in release/2.2 #33230

Closed

saurabh500 merged commit 0db2274 into dotnet:master Nov 6, 2018

karelz added this to the 3.0 milestone Nov 15, 2018

MaximLipnin mentioned this pull request Apr 1, 2019

SQL Server Connection Hangs when Timeout is Set on Connection String mono/mono#13065

Closed

AfsanehR-zz mentioned this pull request Jul 25, 2019

Add additional KeepAlive checks in ManagedSNI for Windows dotnet/SqlClient#70

Closed

cheenamalhotra mentioned this pull request Jan 23, 2020

Introducing 'netcoreapp3.1' DLL with Cross-Platform TCP Keep Alive support dotnet/SqlClient#395

Merged

AfsanehR-zz mentioned this pull request Jan 31, 2020

TCP KeepAlive options having different measurements in Linux and Windows OS. dotnet/runtime#27759

Closed

Add TCPKeepAlive to SqlClient Sockets #33024

Add TCPKeepAlive to SqlClient Sockets #33024

Conversation

AfsanehR-zz commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AfsanehR-zz commented Oct 25, 2018

AfsanehR-zz commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh500 Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AfsanehR-zz Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luigiberrettini Oct 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luigiberrettini Oct 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh500 Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

AfsanehR-zz commented Oct 26, 2018

AfsanehR-zz commented Oct 26, 2018

kburtram commented Oct 31, 2018

stephentoub commented Nov 1, 2018

kburtram commented Nov 1, 2018

AfsanehR-zz commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh500 left a comment

Choose a reason for hiding this comment

AfsanehR-zz commented Nov 2, 2018

AfsanehR-zz commented Nov 2, 2018

AfsanehR-zz commented Nov 2, 2018

AfsanehR-zz commented Nov 3, 2018

AfsanehR-zz commented Nov 3, 2018

saurabh500 commented Nov 3, 2018

AfsanehR-zz commented Nov 5, 2018

AfsanehR-zz commented Nov 5, 2018

jinek commented Apr 13, 2021

stephentoub commented Apr 13, 2021

saurabh500 Oct 25, 2018 •

edited

Loading

AfsanehR-zz Oct 26, 2018 •

edited

Loading

luigiberrettini Oct 27, 2018 •

edited

Loading

luigiberrettini Oct 28, 2018 •

edited

Loading

saurabh500 Oct 29, 2018 •

edited

Loading